Historical string power level clarification#3688
Conversation
Co-authored-by: Jonathan de Jong <jonathandejong02@gmail.com>
| from the string. In these cases, the following formatting of the power level | ||
| string is allowed: | ||
|
|
||
| - a single Base10 integer, no float values or decimal points, optionally with leading zeroes; |
There was a problem hiding this comment.
One thing i need to ask, and mention, is what the range of this integer will be.
Synapse does not correctly validate a string-passed integer, allowing it to become bigger than 64-bit, or the canonical JSON 2^53 limit.
I personally think this latter bit should not be included, and integers should be validated according to the canonical JSON limit, but it is still worth mentioning. (synapse issue: matrix-org/synapse#11873)
There was a problem hiding this comment.
Limits are difficult because this dates all the way back to python2 which had a firm limit, but the bug has persisted through a python3 transition where no practical limits are applied.
This is way out of my depth as a reviewer, so will defer to the server-side folk of the SCT for comment.
There was a problem hiding this comment.
requiring bigints would significantly complicate server implementations, as in many languages using them isn't as simple as in python.
There was a problem hiding this comment.
I would not relax the limit on the integers. Our database adventures found not a single event outside the range. So it should be safe to not also relax the limit on the integer range. If it actually breaks rooms, we could add such language to the spec then?
There was a problem hiding this comment.
I would not relax the limit on the integers.
+1. This whole thing is a balance between not breaking too many existing rooms, whilst not making supporting those existing rooms too onerous for non-Synapse implementations.
Given that we don't know of any rooms which have stringy PLs outside [-2^53, 2^53), requiring non-synapse implementations to support such PLs seems an unnecessary burden.
There was a problem hiding this comment.
@deepbluev7 says:
I suggest we break rooms with PLs outside the range in v6+ and add an appropriate error message to synapse that says to report a bug, if an existing room is broken because of that. And then when someone comes and actually complains about it, we can discuss relaxing the limit as well. We don't need to allow all synapse currently allows, just what is already in the wild. And those are from my search only PLs inside the range, but as strings, in v6+. As long as it is clear how to report, if someone finds a non-conforming room, I think it is fine to break those temporarily.
There was a problem hiding this comment.
(in other words, let's change synapse to reject such PL events if it sees them in the future)
Edit: in room v6+, obviously. room v5 and earlier are kinda stuck with bigint PLs.
There was a problem hiding this comment.
Limits are difficult because this dates all the way back to python2 which had a firm limit, but the bug has persisted through a python3 transition where no practical limits are applied.
I don't think this is accurate. I just did some testing on Python 2:
>>> int("12345689012345678901234567890")
12345689012345678901234567890L
>>> type(_)
<type 'long'>On python 3 this creates:
>>> int("12345689012345678901234567890")
12345689012345678901234567890
>>> type(_)
<class 'int'>The difference is that Python 2 had a separate long class while Python 3 has an int class which expands to any size (essentially long from Python 2). json.dumps seems to work fine (and creates invalid JSON going over the 2 ^ 53 limit... for a number)
tl;dr I think both Python 2 & 3 didn't have a limit.
turt2live
left a comment
There was a problem hiding this comment.
Overall this is a sensible inclusion - comments are mostly regarding spec structure rather than the change itself.
| #### Definitions | ||
|
|
||
| **Required Power Level** \ | ||
| ##### Required Power Level |
There was a problem hiding this comment.
These shouldn't be promoted to headings - the bolding is fine
| Power level events in room versions up to and including room version 9 | ||
| can be optionally represented in string format, in order to maintain | ||
| compatibility with pre-1.0 implementations without breaking existing rooms. | ||
| A homeserver must be prepared to deal with this by parsing the power level | ||
| from the string. In these cases, the following formatting of the power level | ||
| string is allowed: |
There was a problem hiding this comment.
We'll define a range when we pull this out of the s2s spec in favour of the room version specs (this section will get airlifted out of this doc).
For now, the following (or a variation therein) should be fine:
| Power level events in room versions up to and including room version 9 | |
| can be optionally represented in string format, in order to maintain | |
| compatibility with pre-1.0 implementations without breaking existing rooms. | |
| A homeserver must be prepared to deal with this by parsing the power level | |
| from the string. In these cases, the following formatting of the power level | |
| string is allowed: | |
| In order to maintain backwards compatibility with early implementations, | |
| power levels can optionally be represented in string format instead of | |
| integer format. A homeserver must be prepared to deal with this by parsing | |
| the power level from a string. In these cases, the following formatting of the | |
| power level string is allowed: |
There was a problem hiding this comment.
something missing from this paragraph is a pointer to what value we're talking about though. Examples after the grammar would probably be best, demonstrating a scenario or two (kick power level being "12" and a user having PL "42" or something similar).
| from the string. In these cases, the following formatting of the power level | ||
| string is allowed: | ||
|
|
||
| - a single Base10 integer, no float values or decimal points, optionally with leading zeroes; |
There was a problem hiding this comment.
Limits are difficult because this dates all the way back to python2 which had a firm limit, but the bug has persisted through a python3 transition where no practical limits are applied.
This is way out of my depth as a reviewer, so will defer to the server-side folk of the SCT for comment.
| This behaviour is preserved strictly for backward compatibility only. A | ||
| homeserver should take reasonable precautions to prevent users from | ||
| uploading new power level events with string values and must never | ||
| populate the default power levels in a room as string values. |
There was a problem hiding this comment.
| This behaviour is preserved strictly for backward compatibility only. A | |
| homeserver should take reasonable precautions to prevent users from | |
| uploading new power level events with string values and must never | |
| populate the default power levels in a room as string values. | |
| {{% boxes/warning %}} | |
| This behaviour is preserved strictly for backward compatibility only. A | |
| homeserver should take reasonable precautions to prevent users from | |
| sending new power level events with string values and must never | |
| populate the default power levels in a room as string values. | |
| {{% /boxes/warning %}} |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| Power level events in room versions up to and including room version 9 | ||
| can be optionally represented in string format, in order to maintain |
There was a problem hiding this comment.
I don't know if it is worth mentioning but.... room versions 1 to 5 inclusive can unfortunately have float power-levels that get cast to int as well in Synapse. See MSC2540.
Again from the Python docs:
For floating point numbers, this truncates towards zero.
😢
richvdh
left a comment
There was a problem hiding this comment.
@neilalexander are you able to make the requested changes?
It would certainly be great to get this clarified.
|
Replaced by matrix-org/matrix-spec#1082. |
This PR aims to clarify and to define a reasonable standard for handling string power levels in room versions up to and including room version 9.
Related: matrix-org/matrix-spec#853, msc3667.
Preview: https://pr3688--matrix-org-previews.netlify.app